feat: wizard tool selection for runtimes and AI tools#8
Merged
Conversation
Add multi-select steps for dev runtimes (Node.js, Bun, Docker) and AI tools (claude-code, gemini, codex, opencode, ollama) to the init wizard. Selections flow through WizardResult → ProvisionOptions and ToolsConfig, and are provisioned by the Lima provider via a new aitools/provision pipeline. Snapshot logic refactored alongside with expanded tests.
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds optional runtime (Node, Bun, Docker) and AI-tool selection to the CLI/wizard, extends config with a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant LimaProvider
participant VM
User->>CLI: Run wizard / provide flags (runtimes, AI tools)
CLI->>CLI: Build api.ProvisionOptions (flags > config)
CLI->>Config: Persist Config.Tools (optional)
CLI->>LimaProvider: ProvisionVM(ctx, name, provisionOpts)
LimaProvider->>LimaProvider: ProvisionSteps(name, provisionOpts)
note right of LimaProvider: Build baseline steps + optional steps\n(Node/pnpm, Bun, Docker, AI tools)
loop for each ProvisionStep
LimaProvider->>VM: Execute step (pkg install, user setup, tool install)
VM-->>LimaProvider: step success/failure
end
LimaProvider-->>CLI: Provisioning complete
CLI-->>User: VM ready (with selected runtimes/tools)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/vm/lima/snapshot_test.go (1)
24-92:⚠️ Potential issue | 🟡 MinorReplace
nilcontext withcontext.Background()(ort.Context()on Go 1.24+).
golangci-lint/staticcheckSA1012 is flagging thenilcontexts passed toSnapshotClean/RestoreCleanat lines 24, 70, and 92 (and similarnilcalls exist in the non-diffed tests below). Even though these implementations don't currently read fromctx, passingnilis the kind of thing that blows up the first time anyone adds actx.Done()check. Cheap to fix and silences the linter.🛠️ Proposed fix
- if err := p.SnapshotClean(nil, "test-vm"); err != nil { + if err := p.SnapshotClean(context.Background(), "test-vm"); err != nil { t.Fatalf("SnapshotClean() error: %v", err) }Apply the same pattern to the other
nil-context sites in this file. (contextis already imported.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/vm/lima/snapshot_test.go` around lines 24 - 92, Replace nil contexts passed to SnapshotClean and RestoreClean with a real context (use context.Background() or t.Context() if on Go 1.24+) so the calls like p.SnapshotClean(nil, "test-vm") and p.RestoreClean(nil, "test-vm") become non-nil; update all occurrences in this test file (including other nil uses around TestSnapshotClean_RemovesLegacy and TestRestoreClean) to pass a proper context to the methods to satisfy staticcheck SA1012.
🧹 Nitpick comments (2)
cmd/airlock/cli/wizard/questions.go (1)
427-435: Optional: replacecontainsStringwithslices.Contains.
slices.Contains(stdlibslices, Go 1.21+) does exactly this. One-liner call site, no helper to maintain.♻️ Proposed diff
-// containsString reports whether slice contains target. -func containsString(slice []string, target string) bool { - for _, s := range slice { - if s == target { - return true - } - } - return false -}At the call sites:
- result.InstallNode = containsString(runtimes, RuntimeNode) - result.InstallBun = containsString(runtimes, RuntimeBun) - result.InstallDocker = containsString(runtimes, RuntimeDocker) + result.InstallNode = slices.Contains(runtimes, RuntimeNode) + result.InstallBun = slices.Contains(runtimes, RuntimeBun) + result.InstallDocker = slices.Contains(runtimes, RuntimeDocker)Add
"slices"to the import block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/airlock/cli/wizard/questions.go` around lines 427 - 435, The helper function containsString can be removed and replaced with the standard library slices.Contains; update all call sites that call containsString(...) to call slices.Contains(...) instead and add "slices" to the import block of the package, then delete the containsString function declaration to avoid duplication. Ensure the semantics remain the same (slice []string, target string) when swapping calls and run tests/build to confirm imports are correct.cmd/airlock/cli/wizard/mappings.go (1)
212-221: Consider keeping npm-requirement metadata alongsideAITools()to avoid drift.
AIToolRequiresNpmhard-codes the npm-based tool keys in a separateswitch, whileAITools()on lines 225–233 is already the declared single source of truth for tool options. If a new npm-based tool is added toAITools()(or one of the existing keys stops requiring npm), theswitchhere must also be updated, and — per the comment — must additionally stay in sync with the provisioner registry ininternal/vm/lima. That's three places to keep aligned.A small refactor to carry a
RequiresNpm boolonAIToolInfowould collapse the wizard-side duplication to one table, leaving only the cross-package sync with the Lima registry:♻️ Proposed refactor
type AIToolInfo struct { Key string Label string // shown in multi-select ShortLabel string // shown in summary + RequiresNpm bool } -func AIToolRequiresNpm(key string) bool { - switch key { - case AIToolClaudeCode, AIToolGemini, AIToolCodex: - return true - } - return false -} +func AIToolRequiresNpm(key string) bool { + for _, t := range AITools() { + if t.Key == key { + return t.RequiresNpm + } + } + return false +} @@ - {Key: AIToolClaudeCode, Label: "Claude Code (Anthropic)", ShortLabel: "Claude Code"}, - {Key: AIToolGemini, Label: "Gemini CLI (Google)", ShortLabel: "Gemini CLI"}, - {Key: AIToolCodex, Label: "Codex CLI (OpenAI)", ShortLabel: "Codex CLI"}, - {Key: AIToolOpenCode, Label: "OpenCode", ShortLabel: "OpenCode"}, - {Key: AIToolOllama, Label: "Ollama (local LLM runtime)", ShortLabel: "Ollama"}, + {Key: AIToolClaudeCode, Label: "Claude Code (Anthropic)", ShortLabel: "Claude Code", RequiresNpm: true}, + {Key: AIToolGemini, Label: "Gemini CLI (Google)", ShortLabel: "Gemini CLI", RequiresNpm: true}, + {Key: AIToolCodex, Label: "Codex CLI (OpenAI)", ShortLabel: "Codex CLI", RequiresNpm: true}, + {Key: AIToolOpenCode, Label: "OpenCode", ShortLabel: "OpenCode"}, + {Key: AIToolOllama, Label: "Ollama (local LLM runtime)", ShortLabel: "Ollama"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/airlock/cli/wizard/mappings.go` around lines 212 - 221, Refactor to keep npm-requirement with the tool definitions: add a RequiresNpm bool to the AIToolInfo struct and set it on each entry returned by AITools(), then replace the hard-coded switch in AIToolRequiresNpm to look up the tool by key via AITools() (or a small map derived from it) and return that tool's RequiresNpm value; update any callers to use the new field lookup so the npm requirement is maintained in one place (refer to AIToolInfo, AITools, and AIToolRequiresNpm).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/airlock/cli/cli.go`:
- Around line 184-206: mergeWithDefaults currently skips merging the
Config.Tools fields so empty/partial [tools] in airlock.toml override defaults;
update mergeWithDefaults (and ensure Defaults() provides defaults) to copy
missing tool values from defaults into cfg: for boolean fields Node, Bun, Docker
set cfg.Tools.<Field> = defaults.Tools.<Field> when cfg.Tools.<Field> is
false/zero and for the slice AITools set cfg.Tools.AITools =
defaults.Tools.AITools when cfg.Tools.AITools is nil; this aligns
mergeWithDefaults with how VM/Dev/Security/Mounts are merged and fixes Load/CLI
precedence behavior.
In `@cmd/airlock/cli/tui/styles.go`:
- Around line 212-217: extractLogMessage currently uses strings.Index to find
"msg=" which can match inside quoted field values; implement a small key-aware
scanner (e.g., findKeyOutsideQuotes or indexKeyOutsideQuotes) that iterates the
line, tracks whether it's inside double quotes (taking escapes into account),
and returns the index of "msg=" only when found outside quoted regions, then
replace the raw strings.Index call in extractLogMessage with this helper so the
function extracts the actual msg= key's value.
In `@cmd/airlock/cli/wizard/mappings.go`:
- Around line 251-261: The ToProvisionOptions function currently copies
r.AITools verbatim into api.ProvisionOptions, so CLI-provided duplicates
(collected via StringSliceVar) flow through and get persisted by ToConfig into
cfg.Tools.AITools; fix by de-duplicating the AI tools when constructing the
ProvisionOptions (or alternatively immediately after flag collection) —
implement a deterministic dedupe that preserves first-seen order (e.g., iterate
r.AITools with a set to skip repeats) and use that unique slice in
ToProvisionOptions (and ensure ToConfig uses the same deduped list if you choose
CLI-side dedupe).
In `@internal/config/config.go`:
- Around line 63-70: The Defaults() implementation currently turns on Node, Bun,
and Docker causing Load() with no config to auto-provision tools; update
Defaults() so ToolsConfig defaults to all false/empty (Node=false, Bun=false,
Docker=false, AITools=nil or empty slice) to preserve opt-in behavior described
by ToolsConfig; locate the ToolsConfig type and the Defaults() function and
change the returned ToolsConfig values accordingly and ensure Load() relies on
Defaults() without mutating them to true.
In `@internal/vm/lima/aitools.go`:
- Around line 38-47: The npmInstallGlobal function uses fmt.Sprintf("%q", pkg)
which emits a double-quoted string and allows command substitution; replace that
with the repository's single-quote escaping helper (shellEscape from
provider.go) so the pkg is safely single-quoted for the bash -c invocation
(i.e., call shellEscape(pkg) when building the command passed to p.Exec under
npmInstallGlobal); after updating, remove the "fmt" import if it becomes unused.
In `@internal/vm/lima/provision.go`:
- Around line 100-132: The Run closures for optional installs (p.installBun,
p.installDocker and the install variable used for AI tools) currently swallow
errors and always return nil; change each closure to capture the returned error
(e.g., err := p.installBun(...)/err := p.installDocker(...)/err :=
install(...)), and if err != nil write a clear message to stderr (e.g., via
fmt.Fprintf(os.Stderr, ...) or the package logger) including the tool name and
error, then return nil so the overall provision continues; apply this pattern to
the Bun step, the Docker step, and inside the loop where install is used for
opts.AITools (optionally consider adding an Optional flag to api.ProvisionStep
later to surface warnings in the TUI).
---
Outside diff comments:
In `@internal/vm/lima/snapshot_test.go`:
- Around line 24-92: Replace nil contexts passed to SnapshotClean and
RestoreClean with a real context (use context.Background() or t.Context() if on
Go 1.24+) so the calls like p.SnapshotClean(nil, "test-vm") and
p.RestoreClean(nil, "test-vm") become non-nil; update all occurrences in this
test file (including other nil uses around TestSnapshotClean_RemovesLegacy and
TestRestoreClean) to pass a proper context to the methods to satisfy staticcheck
SA1012.
---
Nitpick comments:
In `@cmd/airlock/cli/wizard/mappings.go`:
- Around line 212-221: Refactor to keep npm-requirement with the tool
definitions: add a RequiresNpm bool to the AIToolInfo struct and set it on each
entry returned by AITools(), then replace the hard-coded switch in
AIToolRequiresNpm to look up the tool by key via AITools() (or a small map
derived from it) and return that tool's RequiresNpm value; update any callers to
use the new field lookup so the npm requirement is maintained in one place
(refer to AIToolInfo, AITools, and AIToolRequiresNpm).
In `@cmd/airlock/cli/wizard/questions.go`:
- Around line 427-435: The helper function containsString can be removed and
replaced with the standard library slices.Contains; update all call sites that
call containsString(...) to call slices.Contains(...) instead and add "slices"
to the import block of the package, then delete the containsString function
declaration to avoid duplication. Ensure the semantics remain the same (slice
[]string, target string) when swapping calls and run tests/build to confirm
imports are correct.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a79a25b8-5e78-49d7-bc33-62e9f340e1fa
📒 Files selected for processing (18)
cmd/airlock/cli/cli.gocmd/airlock/cli/tui/styles.gocmd/airlock/cli/wizard/mappings.gocmd/airlock/cli/wizard/mappings_test.gocmd/airlock/cli/wizard/questions.gocmd/airlock/cli/wizard/questions_test.gointernal/api/vm.gointernal/config/config.gointernal/config/config_test.gointernal/config/save.gointernal/vm/lima/aitools.gointernal/vm/lima/config_test.gointernal/vm/lima/provider.gointernal/vm/lima/provision.gointernal/vm/lima/snapshot.gointernal/vm/lima/snapshot_test.gotest/integration/harness_test.gotest/integration/snapshot_test.go
- aitools.go: replace %q (double-quote, still shell-expanded) with shellEscape for npm package names. Prevents command substitution in a hypothetical future caller passing attacker-controlled strings. - provision.go: log optional install failures to stderr instead of silently swallowing them, so users see when Bun/Docker/AI-tool installs fail rather than only noticing later via missing binaries. - provision.go: dedup opts.AITools in ProvisionSteps so CLI duplicates (--ai-tool foo --ai-tool foo) do not queue redundant install steps. Preserves first-seen order. - tui/styles.go: add key-aware indexLogField scanner so extractLogMessage only matches msg= at a token boundary, not inside another field's quoted value.
Owner
Author
|
Addressed CodeRabbit feedback in d27a29f: Fixed
Declined / deferred
|
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/config/config.go (1)
1-1:⚠️ Potential issue | 🟡 MinorRun
gofmt -w .to fix formatting.CI reports
make fmtfailing for this file — likely struct-tag alignment inConfigafter inserting the newToolsfield on Line 24. Please rungofmt -w internal/config/config.gobefore merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` at line 1, The Config struct’s formatting (specifically the newly added Tools field) is misaligned causing gofmt to fail; open the Config type (symbol: Config) and the Tools field declaration, run gofmt -w on the file (or run make fmt) to normalize struct tag alignment and formatting, then stage and commit the formatted file so CI's formatting check passes.
🧹 Nitpick comments (1)
internal/config/config.go (1)
188-200:mergeWithDefaultsforToolsis effectively a no-op.Since
Defaults()now returnsNode/Bun/Docker=falseandAITools=nil, each branch here copies zero/nil onto zero/nil. This matches the opt-in behavior the PR intends, and I see from the PR description that the proper fix (pointer-bools or detect-presence-on-load) is explicitly tracked as follow-up — flagging only so the dead-looking code isn't mistaken for working defaults preservation later.No change requested in this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 188 - 200, The mergeWithDefaults block for Tools (function mergeWithDefaults in internal/config/config.go) is effectively a no-op because Defaults() now sets Node/Bun/Docker to false and AITools to nil; to avoid confusion mark this as intentionally opt-in: add an inline comment or TODO above the Tools merge stating that current behavior is intentional, reference that Defaults() returns false/nil for Tools, and note the planned follow-up to switch to pointer-bools or presence-detection so future readers won’t assume this code is meant to populate defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/config/config.go`:
- Line 1: The Config struct’s formatting (specifically the newly added Tools
field) is misaligned causing gofmt to fail; open the Config type (symbol:
Config) and the Tools field declaration, run gofmt -w on the file (or run make
fmt) to normalize struct tag alignment and formatting, then stage and commit the
formatted file so CI's formatting check passes.
---
Nitpick comments:
In `@internal/config/config.go`:
- Around line 188-200: The mergeWithDefaults block for Tools (function
mergeWithDefaults in internal/config/config.go) is effectively a no-op because
Defaults() now sets Node/Bun/Docker to false and AITools to nil; to avoid
confusion mark this as intentionally opt-in: add an inline comment or TODO above
the Tools merge stating that current behavior is intentional, reference that
Defaults() returns false/nil for Tools, and note the planned follow-up to switch
to pointer-bools or presence-detection so future readers won’t assume this code
is meant to populate defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 209cba44-bfb5-4eb6-aa81-efa4ebbaec18
📒 Files selected for processing (6)
README.mdcmd/airlock/cli/tui/styles.gocmd/airlock/cli/wizard/mappings.gointernal/config/config.gointernal/vm/lima/aitools.gointernal/vm/lima/provision.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/airlock/cli/tui/styles.go
- cmd/airlock/cli/wizard/mappings.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WizardResult→api.ProvisionOptionsandconfig.ToolsConfig; Lima provider provisions via newaitools.go/provision.gopipeline.internal/vm/lima/snapshot.gowith expanded test coverage; tune CWD-basename default naming in the wizard.Principles check
internal/vm/lima/aitools.go) is table-driven — new tools register without editing the switch. Wizard only knows keys viawizard/mappings.go.api.ProvisionOptions; Lima provider implements. No direct Lima import from wizard.mappings.goconstants) shared by wizard, API, and provisioner;AIToolRequiresNpmcentralizes the Node implication.go vet ./...clean,go test ./...380 pass. New coverage inwizard/mappings_test.go,wizard/questions_test.go,internal/vm/lima/snapshot_test.go,internal/config/config_test.go.AI transparency
AI-generated with human review. Used Claude Code to draft wizard steps, provisioner pipeline, and expanded snapshot tests; every line reviewed and tested by author before commit.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
Documentation